Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch compilation from C++17 to C++20. #4347

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Switch compilation from C++17 to C++20. #4347

wants to merge 1 commit into from

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Jan 22, 2024

C++20 is now supported in most major distributions and compilers and there are several features that are useful for us to have. Let's switch to C++20.

@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Jan 22, 2024
@vlstill
Copy link
Contributor

vlstill commented Jan 23, 2024

I started looking at this some time ago in #4297. I was probably too sneaky though. Still not complete, there is quite lot of things that break and I don't have much time for it now. The main problem is clashing operators.

@fruffy
Copy link
Collaborator Author

fruffy commented Jan 23, 2024

I started looking at this some time ago in #4297. I was probably too sneaky though. Still not complete, there is quite lot of things that break and I don't have much time for it now. The main problem is clashing operators.

I can close this PR in favor of yours. I just wanted to see what needs to be done and it seems moving up is non-trivial.

@vlstill
Copy link
Contributor

vlstill commented Jan 23, 2024

I started looking at this some time ago in #4297. I was probably too sneaky though. Still not complete, there is quite lot of things that break and I don't have much time for it now. The main problem is clashing operators.

I can close this PR in favor of yours. I just wanted to see what needs to be done and it seems moving up is non-trivial.

OK. Yes, it does not seem as easy as was the move to C++17.

@fruffy fruffy closed this Jan 23, 2024
@fruffy fruffy deleted the fruffy/C++20 branch January 23, 2024 16:49
@fruffy fruffy restored the fruffy/C++20 branch April 11, 2024 18:01
@fruffy fruffy reopened this Apr 11, 2024
@fruffy fruffy added run-ubuntu18 Use this tag to trigger a Ubuntu-18 CI run. run-sanitizer Use this tag to run a Clang+Sanitzers CI run. run-static Use this tag to trigger static build CI run. labels Apr 11, 2024
@fruffy fruffy marked this pull request as ready for review May 31, 2024 12:41
@fruffy
Copy link
Collaborator Author

fruffy commented May 31, 2024

There is not much missing for C++20 support. It looks like there is just one small failure left.

@vlstill
Copy link
Contributor

vlstill commented May 31, 2024

Hmm, the error is in boost, that might be hard to fix unless it is our bad usage of boost of course :-). Seems to be related to a particular version as it fails just in the sanitizers build.

What are your thoughts on switching? Do you think it is time for that? We don't get many goodies until we bump minimal required GCC, but even some things from GCC 9 are nice. It would also unlock C++20 for downstreams that have newer compilers. On the other hand, we would be having a more experimental setup, with GCC that is far from stable on C++20.

@fruffy
Copy link
Collaborator Author

fruffy commented May 31, 2024

Hmm, the error is in boost, that might be hard to fix unless it is our bad usage of boost of course :-). Seems to be related to a particular version as it fails just in the sanitizers build.

The boost version should be the same for Ubuntu 20.04 but this compiler is Clang. Maybe that is the reason.

What are your thoughts on switching? Do you think it is time for that? We don't get many goodies until we bump minimal required GCC, but even some things from GCC 9 are nice. It would also unlock C++20 for downstreams that have newer compilers. On the other hand, we would be having a more experimental setup, with GCC that is far from stable on C++20.

I am happy to switch, there are some folks that have been waiting for us to upgrade to C++20 already. We can try to be conservative with the features we introduce and keep Ubuntu 20.04 as the minimum supported version until it hits EOL.

@fruffy fruffy changed the title Switch from C++17 to C++20. Switch compilation from C++17 to C++20. May 31, 2024
@fruffy
Copy link
Collaborator Author

fruffy commented May 31, 2024

Hmm, the error is in boost, that might be hard to fix unless it is our bad usage of boost of course :-). Seems to be related to a particular version as it fails just in the sanitizers build.

Cherrypicking #4663 fixes the issue. There were two more small fixes required which I can fork into a separate PR.

@vlstill
Copy link
Contributor

vlstill commented Jun 3, 2024

Hmm, the error is in boost, that might be hard to fix unless it is our bad usage of boost of course :-). Seems to be related to a particular version as it fails just in the sanitizers build.

Cherrypicking #4663 fixes the issue. There were two more small fixes required which I can fork into a separate PR.

Personally I'm OK with the operator fixes to be there, but the boost should be separate.

@vlstill
Copy link
Contributor

vlstill commented Jun 3, 2024

I've just notice that docs/C++.md still mentions C++11. No point fixing it to 17 now, but should be fixed when we switch to C++20.

@fruffy
Copy link
Collaborator Author

fruffy commented Jun 3, 2024

Personally I'm OK with the operator fixes to be there, but the boost should be separate.

Yes, the changes were for testing purposes, I will move them into separate PRs.

@fruffy fruffy force-pushed the fruffy/C++20 branch 3 times, most recently from 63d7c31 to 07beb28 Compare June 3, 2024 18:08
@fruffy
Copy link
Collaborator Author

fruffy commented Jun 3, 2024

It looks like there is very little that can be done to fix the compilation issue sans fixing boost. I am not sure why this pops up with clang only though.

@fruffy
Copy link
Collaborator Author

fruffy commented Jul 3, 2024

@asl Any idea how to workaround this Boost C++20 bug?

https://stackoverflow.com/a/67702145

It's the only thing blocking compilation for C++20 for our CI. Otherwise, #4663 is required.

@vlstill
Copy link
Contributor

vlstill commented Jul 3, 2024

@asl Any idea how to workaround this Boost C++20 bug?

https://stackoverflow.com/a/67702145

It's the only thing blocking compilation for C++20 for our CI. Otherwise, #4663 is required.

Does this only show on the combination of versions of Clang and Boost on Ubuntu 20.04? Do we know why it does not show with GCC on the same OS? That sounds quite weird for me frankly as it seems like problem in interaction between standard library and boost, not compiler and boost (and both GCC and clang should presumably use the same version of libstdc++ on the system). If it was just Clang problem, I would suggest trying to move the sanitizers to Ubuntu 22.04. Ubuntu 20 is quite old now after all.

That said, we claim to support Clang 6, that should probably be revisited too, we don't test with it. I doubt anyone would require Clang unless on macOS. We should keep it supported, we need the clang-related tools like sanitizers and tidy, but we can probably be a little bit more aggressive in dropping support for old versions.

@asl
Copy link
Contributor

asl commented Jul 3, 2024

I tend to agree with @vlstill as it seems to be some bad interaction with standard library. Anyway, if boost uses things that were deprecated in C++17 and removed in C++20, then there is no way to workaround rather than patch / upgrade the sources.

Or we can just get rid boost::format so the problem will go away

PS: There are reports about overheads (runtime and compile time) of std::format, so we might evaluate the things here...

@fruffy
Copy link
Collaborator Author

fruffy commented Jul 3, 2024

Or we can just get rid boost::format so the problem will go away

Would be nice, but quite tough because of the difference in formatting... not even sure how you could start here.

PS: There are reports about overheads (runtime and compile time) of std::format, so we might evaluate the things here...

What about fmt? Will also be much easier to replace with std::format in the future.

@asl
Copy link
Contributor

asl commented Jul 3, 2024

What about fmt? Will also be much easier to replace with std::format in the future.

it is nice, yes. But probably we'd need to check / benchmark different solutions

@fruffy
Copy link
Collaborator Author

fruffy commented Jul 3, 2024

What about fmt? Will also be much easier to replace with std::format in the future.

it is nice, yes. But probably we'd need to check / benchmark different solutions

Hmm, I think at least for the usage of boost::format in the compiler most of the calls are used for debugging/warnings/errors, which are not that performance-critical, no? Going with the standard solution is likely fine here.

@asl
Copy link
Contributor

asl commented Jul 3, 2024

Hmm, I think at least for the usage of boost::format in the compiler most of the calls are used for debugging/warnings/errors, which are not that performance-critical, no? Going with the standard solution is likely fine here.

The complains were both for compile time and for runtime IIRC. This is why I said we need to check :)

@vlstill
Copy link
Contributor

vlstill commented Jul 4, 2024

Apparently boost 1.74 is the one to fix this: https://www.boost.org/users/history/version_1_74_0.html

Format:
Correct allocator usage (fixes C++20 compilation). (Glen Fernandes)

So that means the one in Ubuntu 22.04 is OK (that is 1.74), but the one in 20.04 (1.71) is bad.

@vlstill
Copy link
Contributor

vlstill commented Aug 18, 2024

The reason this fails in the sanitizer build has actually nothing to do with clang and everything to do with GCC 10, which is installed in that image, but not in the one used for normal Ubuntu 20.04 build. It is GCC 10 that first removes the overload that boost::format requires. So that is why it failed only there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser) run-sanitizer Use this tag to run a Clang+Sanitzers CI run. run-static Use this tag to trigger static build CI run. run-ubuntu18 Use this tag to trigger a Ubuntu-18 CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants